Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

infra(ci): run against playground #3095

Open
wants to merge 28 commits into
base: next
Choose a base branch
from
Open

Conversation

mshima
Copy link
Contributor

@mshima mshima commented Sep 7, 2024

Main types index.d.ts doesn’t exist.
Fix types to match '.' exports.

ref: jhipster/generator-jhipster#27179

@mshima mshima requested a review from a team as a code owner September 7, 2024 14:13
Copy link

netlify bot commented Sep 7, 2024

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 356e3ae
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/66e03332d2c0220008ede2e5
😎 Deploy Preview https://deploy-preview-3095.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mshima mshima changed the title chore: fix main types fix: fix main types Sep 7, 2024
Copy link

codecov bot commented Sep 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (18ab2c7) to head (356e3ae).
Report is 18 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3095      +/-   ##
==========================================
+ Coverage   99.96%   99.97%   +0.01%     
==========================================
  Files        2776     2776              
  Lines      226260   226260              
  Branches      945      591     -354     
==========================================
+ Hits       226183   226207      +24     
+ Misses         77       53      -24     

see 1 file with indirect coverage changes

@Shinigami92 Shinigami92 added the s: invalid This doesn't seem right label Sep 7, 2024
@mshima
Copy link
Contributor Author

mshima commented Sep 7, 2024

Same fix is applied in #3094.
Alternative fix is applied in #3093 (removes types from root)

The above PRs tries to fix CommonJS types too, while this fixes esm types.

@Shinigami92
Copy link
Member

you can checkout https://github.com/faker-js/playground and test your changes

@mshima
Copy link
Contributor Author

mshima commented Sep 7, 2024

you can checkout https://github.com/faker-js/playground and test your changes

Why don’t integrate playground on ci tests?

@Shinigami92
Copy link
Member

you can checkout faker-js/playground and test your changes

Why don’t integrate playground on ci tests?

PR welcome, go and try for it

@mshima mshima marked this pull request as draft September 7, 2024 17:45
@Shinigami92
Copy link
Member

@mshima thanks, this is a huge step forward and I wished for something like this a long time ❤️
you need to rebind/relink the repo faker to the playground test run
so something like modifying pnpm override before installing the playground deps

@mshima mshima marked this pull request as ready for review September 7, 2024 17:55
@mshima
Copy link
Contributor Author

mshima commented Sep 7, 2024

@Shinigami92 I am on mobile now, so I cannot rebase.
I think it’s only missing steps names now.
Feel free to modify the PR.

Playground is passing 🚀

@mshima

This comment was marked as outdated.

package.json Outdated Show resolved Hide resolved
@mshima mshima mentioned this pull request Sep 8, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Sep 8, 2024

Please note that this will create a bi-directional dependency between two separate repositories.

@Shinigami92
Copy link
Member

Please note that this will create a bi-directional dependency between two separate repositories.

Do you see any problem with this? IMO it's fine, because playground is in out org as well.

Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we activate the playground for all PRs why would we need check-release-pr.yml anymore?

Do you see any problem with this? IMO it's fine, because playground is in out org as well.

We had a reason why we didn't do this previously. Now a random contributor might need to apply changes in the playground onto of the changes in faker when they work a breaking change feature.

.github/workflows/playground.yml Outdated Show resolved Hide resolved
@mshima
Copy link
Contributor Author

mshima commented Sep 10, 2024

IMO running playground in CI:

We could do that but it has a side effect, that all features that are used in the playground that might experience breaking changes need to be updated to the new feature before it is available as a dependency. This is also the case for the releasenPR but that those are even rarer.

  • the workflow can be ignored if it's not required to pass.
  • contributor will be warned about breaking changes or unwanted breaking changes.

Moving playground to the main repository can be considered too.

@Shinigami92
Copy link
Member

  • the workflow can be ignored if it's not required to pass.
  • contributor will be warned about breaking changes or unwanted breaking changes.

I agree 👍 we can mark the CI check as "not required" and if it breaks something, we are just "informed"

@mshima mshima dismissed stale reviews from prisis and Shinigami92 via 97237d9 September 10, 2024 11:47
Shinigami92
Shinigami92 previously approved these changes Sep 10, 2024
@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision labels Sep 12, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Sep 12, 2024

I agree 👍 we can mark the CI check as "not required" and if it breaks something, we are just "informed"

Please note that that will only work once and will cause confusion for future PRs (that will fail the CI even if they didn't break anything because next is already "broken").

I would rather move the playgrounds inside the main repo to avoid these bidirectional dependencies.

@ST-DDT ST-DDT added this to the vAnytime milestone Sep 12, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Sep 12, 2024

Team Decision

We don't want to add bi-directional dependencies between the playground and the faker repo.
We think that moving the playground to the main repo is the best solution for that, but that would require extensive work e.g. converting the repo into a mono-repo, what we don't want to do yet.

We would like to still run the pipeline on demand somehow e.g. using a comment, but we are currently unsure about the exact expectation we have for the pipeline. We will talk about this again in a future team meeting.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 12, 2024

If you have an idea, on how to move the playground to the main repo feel free to open a PR with a suggestion.

@ST-DDT ST-DDT added needs rebase There is a merge conflict s: on hold Blocked by something or frozen to avoid conflicts and removed s: needs decision Needs team/maintainer decision labels Sep 14, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Sep 19, 2024

We are looking into restructuring our project setup to solve this issue. For now, you don't have to update this PR.
Thanks for bringing this to our attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: infra Changes to our infrastructure or project setup needs rebase There is a merge conflict p: 1-normal Nothing urgent s: on hold Blocked by something or frozen to avoid conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants